New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(nextjs): Stop excluding withSentryConfig
from serverless bundles
#6267
fix(nextjs): Stop excluding withSentryConfig
from serverless bundles
#6267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved to unblock but the yarn.lock change should be adressed!
// nft file manifests nextjs generates. (See the code dealing with the `TraceEntryPointsPlugin` below.) So that we don't | ||
// crash people, we therefore need to make sure nothing tries to either require this file or import from it. Setting | ||
// this env variable allows us to test whether or not that's happened. | ||
process.env.SENTRY_WEBPACK_MODULE_LOADED = 'true'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l/m: I just wanna mention that this doesn't actually set an environment variable. This just places the SENTRY_WEBPACK_MODULE_LOADED
property on the global process.env
object - which is more or less the same as doing global.SENTRY_WEBPACK_MODULE_LOADED = true
. It's just that the child_process
api will pick up the process.env
object if not configured otherwise.
I don't know if we want to make that differentiation to avoid ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't totally see the distinction you're making. process.env
is the env for the running node process. Sure, there's an env in the shell in which the script is running, but AFAIK there's no way to modify that from within node, so I don't think that's how anyone would interpret this comment, do you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather just set a global variable here, then we can also use a boolean instead of a string.
Setting on process.env
has some behaviours that maybe someone might not realize, on windows things become case insensitive (process.env.FOO
=== process.env.foo
) and everything on process.env
gets coerced to strings when the getter is called.
Since we don't need the above two behaviours, I would rather just set a mutable global. More obvious what is happening to users, and also allows us to lift the comment up from the usage of the variable to the declaration of the variable (cleaning up the function scope a little).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering Abhi I changed it to be a global in 728562b
"@sentry/core" "7.20.0" | ||
"@sentry/types" "7.20.0" | ||
"@sentry/utils" "7.20.0" | ||
"@sentry/browser@7.20.1": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: We should merge master into this branch. Yarn lock shouldn't have any changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the test app in the build process tests, not our main yarn.lock. I think I get why Kamil set his integration test app to not use a lockfile, because this is going to happen every time we bump a version, but that's handlable in a separate PR. I can pull this bit out of this PR if you feel strongly about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I am dumb sorry
// to test whether or not the file is required instead. | ||
|
||
it('imports from `webpack.ts` if `isBuild` returns true', () => { | ||
jest.isolateModules(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
til
0b68c8f
to
52faf56
Compare
52faf56
to
a9af4aa
Compare
Hey something in this release has meant that our application stopped sending events, trying to track it down further |
@isaac-martin Is this also the case for the newest version of the SDK? In the release that included this PR, we broke the sending of events in dev mode, but that should be fixed in |
My application stopped sending events after I upgraded to ✅ = Works fine 7.13.0 ✅ (before upgrade) |
@hiramhuang Hi, could you open an issue describing your problem with logs and your next.js configuration? |
In #6058, we modified the webpack config we apply to users' builds in the nextjs SDK in order to exclude build-time files from being included in the dependency manifests nextjs creates for each page file, which in turns prevents them from being bundled in the serverless functions into which vercel turns users' routes. Unfortunately, we were just a touch too aggressive, in that we excluded
withSentryConfig.ts
, even though it's needed at runtime when nextjs loadsnext.config.js
.This fixes that by making two key changes:
withSentryConfig.ts
and all of its dependencies, we instead excludewebpack.ts
and dependencies.withSentryConfig.ts
not try to requirewebpack.ts
at runtime (when it won't be there), we now conditionally require it, only at buildtime.Notes:
withSentryConfig
was about to get longer, I split it out into a helper function before adding in the conditionalrequire
call.webpack.ts
is now only conditionally required, rollup doesn't automatically pick it up, so it's been added explicitly to the nextjs rollup config.webpack.ts
has been required, it now sets an env variable noting that it's been loaded.index.ts
was renamed towithSentryConfig.ts
. This PR does the same thing for the corresponding test file.Fixes #6265.